Add Setup DVC Section #3655

julieg18 · 2023-04-07T19:31:45Z

moves setup dvc logic from experiments section to new dvc section

Demo

Screen.Recording.2023-04-10.at.4.29.04.PM.mov

Part of #3434

webview/src/setup/components/Dvc.tsx

julieg18 · 2023-04-10T22:11:09Z

webview/src/setup/components/App.test.tsx

-        type: MessageFromWebviewType.INITIALIZED
-      })
-      expect(mockPostMessage).toHaveBeenCalledTimes(1)
+  it('should send the initialized message on first render', () => {


I'm assuming this test was supposed to be in App instead of Experiments.

julieg18 · 2023-04-10T22:14:14Z

webview/src/setup/components/Experiments.tsx

+        <p>DVC needs to be setup before you can access experiments.</p>
+        <Button
+          onClick={() =>
+            setSectionCollapsed({


Other options could be:

only opening up the new section and not collapsing the current one

Take off buttons entirely and just reference going to other section.

Current choice is opening the section we want the user to go to and collapsing all other sections.

Could hide or "grey out" the sections that are currently unavailable.

webview/src/setup/components/App.test.tsx

julieg18 · 2023-04-10T22:17:12Z

webview/src/setup/components/App.tsx

+      >
+        <Experiments
+          isDvcSetup={
+            projectInitialized && Boolean(cliCompatible) && !needsGitCommit


Is there a better way to check for if DVC is setup properly?

Previously we discussed passing a "step" variable from the extension to let the webview know which screen to show. This would mean that the user could navigate between screens. Not sure how much sense that makes for the setup process.

Also, I don't think we use Boolean(variable) anywhere as opposed to !!variable. Although maybe we should turn on https://eslint.org/docs/latest/rules/no-implicit-coercion

extension/package.json

mattseddon

We need to fix the redirects before merging.

The separation between the sections needs to be well-defined. After setup is complete I think we want to show the details of where the CLI has been located much like the tooltip that we show for the status bar item:

vscode-dvc/extension/src/status.ts

Line 107 in 21ef1dd

private getEnvDetails() {

Seeing as we have a lot more room than a tooltip and we can make things far more obvious we want to show the version of the CLI/Min required version etc as this is very useful information that we can use to get people "unstuck". We can also go in the direction of removing the toast messages and moving them into the setup screen. Toast messages are here:

vscode-dvc/extension/src/cli/dvc/discovery.ts

Line 1 in 21ef1dd

import {

mattseddon · 2023-04-10T22:57:35Z

webview/src/setup/components/App.tsx

+      >
+        <Experiments
+          isDvcSetup={
+            projectInitialized && Boolean(cliCompatible) && !needsGitCommit


Previously we discussed passing a "step" variable from the extension to let the webview know which screen to show. This would mean that the user could navigate between screens. Not sure how much sense that makes for the setup process.

Also, I don't think we use Boolean(variable) anywhere as opposed to !!variable. Although maybe we should turn on https://eslint.org/docs/latest/rules/no-implicit-coercion

mattseddon · 2023-04-10T22:59:40Z

webview/src/setup/components/App.tsx

          isPythonExtensionInstalled={isPythonExtensionInstalled}
          needsGitInitialized={needsGitInitialized}
          needsGitCommit={needsGitCommit}
          projectInitialized={projectInitialized}
          pythonBinPath={pythonBinPath}
+          hasData={hasData}


[Q] What does hasData have to do with setting up the CLI? Isn't this for experiments only? Same question for needsGitCommit?

Do data status and plots diff work or throw an error when there are no commits?

[Q] What does hasData have to do with setting up the CLI? Isn't this for experiments only?

hasData is used for when the DVC section knows what to do with the Show Experiments button that appears when Setup is complete. Though we could rename the prop to make things more clear!

Same question for needsGitCommit?

Makes sense! Will move the needsGitCommit logic to Experiments.

webview/src/setup/components/Dvc.tsx

mattseddon · 2023-04-10T23:02:39Z

webview/src/setup/components/Dvc.tsx

+                  studio: true
+                })
+        }
+        text="Show Experiments"


[C] As well as having the button I think we would want to close the section and open the other ones when "setup completes".

Good idea! Though not sure how to implement it. I thought I could do this with an effect hook, but that breaks SETUP_SHOW_STUDIO and SETUP_SHOW_EXPERIMENTS. Will put some more thought into this one and do it in a followup!

mattseddon · 2023-04-10T23:03:56Z

webview/src/setup/components/Experiments.tsx

+        <p>DVC needs to be setup before you can access experiments.</p>
+        <Button
+          onClick={() =>
+            setSectionCollapsed({


Could hide or "grey out" the sections that are currently unavailable.

julieg18 · 2023-04-11T16:11:47Z

Thanks for the reviews, everyone!

What's Left To Do:

Hide or "grey out" the sections that are currently unavailable. (Add Setup DVC Section #3655 (comment))
Have a section automatically close and open another when setup finished (Add Setup DVC Section #3655 (comment))
Have DVC section show more details about the dvc cli version (number, location, etc.) in the "Setup Complete" screen (Add Setup DVC Section #3655 (review))
Move some of dvc setup messaging that is shown in toasts to the DVC section (Add Setup DVC Section #3655 (review))

All of these items are going to be in separate prs since they are going to require extra reviews. I'm currently working on the first one, but I think we're good to merge! Will wait a bit to make sure there are no objections :)

codeclimate · 2023-04-11T19:42:27Z

Code Climate has analyzed commit 3eaf979 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	1

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 94.9% (0.0% change).

View more on Code Climate.

Add Setup DVC Section

87f0804

julieg18 added the product PR that affects product label Apr 7, 2023

julieg18 self-assigned this Apr 7, 2023

codeclimate bot reviewed Apr 7, 2023

View reviewed changes

webview/src/setup/components/Dvc.tsx Show resolved Hide resolved

webview/src/setup/components/Dvc.tsx Outdated Show resolved Hide resolved

webview/src/setup/components/Dvc.tsx Show resolved Hide resolved

julieg18 added 5 commits April 10, 2023 16:16

Add tests

06a52f1

Adjust walkthrough

c76f52c

Review text

d529a33

Add test for experiments setup dvc button

72b4576

Add experiments loading project test

7bb4c4d

julieg18 marked this pull request as ready for review April 10, 2023 22:09

julieg18 requested review from mattseddon and sroy3 as code owners April 10, 2023 22:09

julieg18 requested a review from shcheklein April 10, 2023 22:09

julieg18 commented Apr 10, 2023

View reviewed changes

mattseddon reviewed Apr 10, 2023

View reviewed changes

extension/package.json Show resolved Hide resolved

mattseddon approved these changes Apr 10, 2023

View reviewed changes

sroy3 approved these changes Apr 11, 2023

View reviewed changes

julieg18 added 3 commits April 11, 2023 10:17

Housekeeping

d07b1b4

Have "Show Experiments" redirect to dvc setup

2960a29

Move git commit logic to experiments

395f31f

Merge branch 'main' into add-setup-dvc-section

3eaf979

julieg18 merged commit 0d8f1db into main Apr 11, 2023

julieg18 deleted the add-setup-dvc-section branch April 11, 2023 19:52

This was referenced Apr 11, 2023

Disable setup section if unavailable #3671

Closed

Consolidate onboarding, connect to Studio, settings & walkthrough #3434

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Setup DVC Section #3655

Add Setup DVC Section #3655

julieg18 commented Apr 7, 2023 •

edited

Loading

julieg18 Apr 10, 2023

julieg18 Apr 10, 2023

mattseddon Apr 10, 2023

julieg18 Apr 10, 2023

mattseddon Apr 10, 2023

mattseddon left a comment

mattseddon Apr 10, 2023

mattseddon Apr 10, 2023

mattseddon Apr 10, 2023

julieg18 Apr 11, 2023

mattseddon Apr 10, 2023

julieg18 Apr 11, 2023

mattseddon Apr 10, 2023

julieg18 commented Apr 11, 2023

codeclimate bot commented Apr 11, 2023

Add Setup DVC Section #3655

Add Setup DVC Section #3655

Conversation

julieg18 commented Apr 7, 2023 • edited Loading

Demo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattseddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

julieg18 commented Apr 11, 2023

codeclimate bot commented Apr 11, 2023

julieg18 commented Apr 7, 2023 •

edited

Loading